Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Control Hub Module #68

Open
wants to merge 83 commits into
base: main
Choose a base branch
from
Open

Control Hub Module #68

wants to merge 83 commits into from

Conversation

LandryNorris
Copy link
Contributor

@LandryNorris LandryNorris commented May 25, 2023

This PR needs a non-production version of the RC app.

This PR adds a control-hub module that allows the user to control a REV control hub via WiFi or USB.

@LandryNorris
Copy link
Contributor Author

The Control Hub must be running a RC app with this commit in order to properly show up in the list

@LandryNorris
Copy link
Contributor Author

LandryNorris commented May 25, 2023

I currently have it set to timeout after 1 second when checking if a control hub is present. If there's not a control hub, this adds an extra second to the list command. We may want to tune this timeout in the future (or better yet, add it as a parameter to the discovery method)

@LandryNorris LandryNorris marked this pull request as ready for review May 30, 2023 14:51
@LandryNorris
Copy link
Contributor Author

A WiFi-connected Control Hub will not have a serialNumber, so it isn't conformant to the ParentRevHub interface. I feel like conceptually it is a parent, and can indeed have children, so we'll need a better solution.

@LandryNorris
Copy link
Contributor Author

It may be good to create a ParentUsbHub interface that has the serialNumber, and add a isParentUsbHub method?

@NoahAndrews
Copy link
Member

NoahAndrews commented May 30, 2023

A WiFi-connected Control Hub will not have a serialNumber, so it isn't conformant to the ParentRevHub interface. I feel like conceptually it is a parent, and can indeed have children, so we'll need a better solution.

We could add the serial number to rcInfo.json using Device.getSerialNumber() so that we can expose it correctly in this library.

packages/control-hub/package.json Outdated Show resolved Hide resolved
packages/control-hub/package.json Outdated Show resolved Hide resolved
packages/control-hub/package.json Outdated Show resolved Hide resolved
packages/control-hub/package.json Outdated Show resolved Hide resolved
packages/control-hub/src/ControlHub.ts Outdated Show resolved Hide resolved
packages/control-hub/tsconfig.json Outdated Show resolved Hide resolved
packages/expansion-hub/src/RevHubType.ts Outdated Show resolved Hide resolved
packages/sample/package.json Outdated Show resolved Hide resolved
packages/sample/package.json Show resolved Hide resolved
packages/sample/src/HubStringify.ts Outdated Show resolved Hide resolved
@LandryNorris LandryNorris changed the title Feature/control hub Control Hub Module May 30, 2023
@LandryNorris
Copy link
Contributor Author

One thing to note about this PR is that other PRs change the expansion hub interface. When creating stubs for commands, I preferred the modified methods (essentially what the interface should look like when all PRs are merged). This means that several PRs have changes this one will need.

Copy link
Member

@NoahAndrews NoahAndrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few more comments, but there's still unresolved ones.

README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current wording implies that adbkit is listed as an actual optional dependency, which is not the case.

"@rev-robotics/rev-hub-core": "*"
},
"peerDependencies": {
"get-port": "6.x"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get-port is no longer used by this package, right? We just want to document how to use it with adbkit to set up port forwarding.

import getPort from "get-port";
import { ControlHub } from "./ControlHub.js";

export async function openConnectedControlHub(): Promise<ControlHub | undefined> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be named something like openControlHub, and accept an IP address and httpPort, with default values of 192.168.43.1 and 8080.

@LandryNorris LandryNorris marked this pull request as draft July 11, 2023 20:14
@NoahAndrews
Copy link
Member

See also WIP changes in this commit: e0b79e9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants